feat(dispatch-data-to-store): add function overloads and allow SSOT by using selector #165
Conversation
| store.dispatch(dispatchAction.update({ payload })); | ||
| switch (dispatchType) { | ||
| case 'set': { | ||
| return payload instanceof Array |
There was a problem hiding this comment.
You're making too big of an assumption here. Yes, ngrx strongly suggests using the entity store for arrays, but it's not mandatory. So people are still allowed to use the regular approach to use arrays. This package facilitates the process of ngrx, so we cannot prevent people from using it like that.
These checks will have to go, because they're too strongly set.
| ); | ||
| } | ||
| case 'add': { | ||
| if (!(payload instanceof Array)) { |
There was a problem hiding this comment.
This check is also not really correct. If you would have an array of arrays in your store, you can no longer use the add method.
There was a problem hiding this comment.
How so? An array of an array is still of type Array. Since we negate the result and then throw an error, the user is forced to provide an array when using add. An array of arrays would most definitely pass this check and be dispatched to the store. 🙂
[[]] instanceof Array // trueThe type BaseStoreAsset also does not support the add method, hence the reason for this aggressive error throwing.
There was a problem hiding this comment.
True, I don't remember what I was referring to tbh 😅
|
|
||
| // Wouter: Get the value recently set to the store. This is done to keep the Store as SSOT. | ||
| return store.select( | ||
| payload instanceof Array |
There was a problem hiding this comment.
I think it's better to run a check on the selectAction rather than on the payload, because again, that's not possible with how the store works.
You'll be better off doing something like selectAction['selectAll'] ? selectAction.selectAll : selectAction.select
| }), | ||
| // Iben: Catch the error and dispatch it to the store | ||
| // Iben: Catch the error and dispatch its to the store |
There was a problem hiding this comment.
Why its? 😅 You want to dispatch the error to the store, aka it
There was a problem hiding this comment.
Of course, no idea how that got changed! 😅
| ); | ||
| } | ||
| case 'update': { | ||
| if (payload instanceof Array) { |
There was a problem hiding this comment.
Perhaps I am incorrect here, but shouldn't the payload of the update not be an Array? (sorry, I can only reference 1 line, apparently 🤷)
Type BaseStoreAsset also does not support an update, I presume there is a reason that was initially left out of its operators?
| store.dispatch(dispatchAction.add({ payload })); | ||
| } else { | ||
| store.dispatch(dispatchAction.update({ payload })); | ||
| switch (dispatchType) { |
There was a problem hiding this comment.
Given that a switch case or an if else structure in this case makes no difference, I'd keep it the way it is if it isn't necessary to change it.
Changing these kinds of things in a PR without real cause makes it harder to review PRs, because you have to first try to figure out why it was done in relation to the changes, and then realize it wasn't really needed to do so.
| ): Observable<null>; | ||
| export function dispatchDataToStore<DataType = any>( | ||
| dispatchAction: BaseStoreActions<DataType>, | ||
| selectAction: null | undefined, |
There was a problem hiding this comment.
I'm not that familiar with these overloads, but what will be the effect of putting the selectAction here on the existing implementation?
It's one thing that the result of this PR forces you to change the return types, but completely changing the order of this implementation forcing you to pass undefined as a second parameter would make it a lot harder to work wit.
I'd personally expect the overload to be either with the dispatchAction and one without it?
There was a problem hiding this comment.
Yes, that would be possible, I presume! 👍
|
To address the comments regarding the errors that are thrown here in the dispatchDataToStore. I've checked but basically came to the conclusion that all of these errors that are thrown are unnecessary.
In summary, these errors are not needed and if we want to improve the behaviour here, it should be through typing first and foremost. |
WHeirstrate
force-pushed
the
feat/ngx-store/dispatch-data-to-store
branch
from
089a236
to
dea1c39
Compare
Explanation
The SSOT principle was not properly respected. The
dispatchDataToStoreutil will not return anything in and of itself, a selector has to be provided. If not provided, the method will returnObservable<null>.Examples
The function overloads make sure that something like this will not be allowed:
The error will indicate that no contract of any overload matches. The
amountis of type number, which indicates aBaseStoreAsset<number>, while the data provided is of typeArray. The overloads will prevent this from passing, whereas previously this would fail to set the data in the store.The error looks like this:
The function overloads also make sure a
BaseStoreAssetcannot haveupdateoraddas dispatchType, for they are only available onEntityStoreAssets.Possible improvements
Moving the function overloads to a type definition file
dispatch-data-to-store.util.d.ts, for example.